We're doing a best effort job of showing possible values when the inclusion validator is simple enough, but we shouldn't go and call random code, this is called in a view after all.

I don't understand this point. I wouldn't qualify this as random code, it's very explicitly the proc to generate the options and it should be fast and reliable, since it's meant to be used in validations.

We have a way to extend the form by adding a maintenance_tasks/tasks/custom partial in the application. (It's not documented BTW sorry). I guess you could use this to conditionally add fields, hide the previous fields for an attribute, if you wanted to go down that route.

This seems very convoluted. Using the partial is only really useful to add fields. How would it work to hide previous fields?

If we were to say that the standard way of customizing fields is through overriding, we should make it easy in the gem, by making a dedicated _field partial or introducing some override mechanism on the fields, like callbacks.

I think it's problematic that some validations can now break your views or make choices inaccessible.

I don't understand this point. What would break? Are you referring to when it has more than 1000 items? If so, I replied elsewhere.

There should be some kind of opt-in at the very least.

As a compromise, I would be ok with a MaintenanceTasks.max_inclusion_items config that defaults to 0 (disabled behaviour).

Or maybe it's just not something that should be done in the engine. We do mention "Normally maintenance tasks are ephemeral, so they are used briefly and then deleted." in the README. The initial use case of wanting to list the application records feels like something that might be built in the application directly instead in some kind of admin interface, if it needs to access any and all models at any time.

I would argue that just because the gem is not intended for permanent tasks isn't a reason to not support making that a viable option.

Otherwise one can always do Rails.application.eager_load!; ActiveRecord::Base.descendants in a console and stick the result in the validation of the task's param (since the task is temporary, it should be fine).

We have cases with ~100 classes to pick from. This quickly becomes noise, a maintenance nightmare, or an annoyance.

Or just live with the fact that the form will not include a dropdown for the model, the target user for the engine being a developer, they should be able to deal with that situation.

This increases the chances of errors.

👉🏼 I think my most important point is that ActiveModel is designed to support dynamic inclusion constraints, but this gem decided to opt-out of that support. I understand that stateful constraints can't be supported, but I don't see why we shouldn't support stateless lambdas.

As a compromise, I would be ok with a MaintenanceTasks.max_inclusion_items config that defaults to 0 (disabled behaviour).

I think that would be good.

I would argue that just because the gem is not intended for permanent tasks isn't a reason to not support making that a viable option.

Yes because there are a thousand no's for every yes. ☺️
First, we added params because this gem replaced enqueuing jobs from migrations, and people were passing arguments to the jobs. And it's useful to be able to test your task on a reduced amount of data without having to redeploy. But then we added dropdowns because if it's easy to do and it makes it easier on the user, why not. And here we are, adding another tweak to that simple thing and adding an arbitrary limit that needs configuration (in part to prevent regressions).
I hope you can see how supporting out-of-scope features tends to creep up.

I think my most important point is that ActiveModel is designed to support dynamic inclusion constraints, but this gem decided to opt-out of that support.

We support it just fine, we're just not making it easy for the user by providing the potential values as a dropdown.

I understand that stateful constraints can't be supported, but I don't see why we shouldn't support stateless lambdas.

So what are stateful constraints by the way? Lambdas that take the record? Also in: :some_method? We do have the object handy (although it's only initialized with its defaults obviously):

I guess they raise more issues, but I wonder why we wouldn't support that as well at this point.

Do we need to deal with potential exceptions raised from the lambda? Before we were just getting a value from the task, now we're calling code, so I guess there's more potential for failure here.

We should also document the dropdown feature (and the scope of the support for views with this PR) in the README as well. I guess that could be an indicator that the feature is too complex if we need too many words to explain it. Before: "if you have an inclusion validator on your parameter, if the allowed values is a Array, it will be used to build a dropdown". Now we'll need to add some more words.

Also just realized that if there's an if: or unless: on a validation, we're not respecting that (and ignoring the validation altogether), and always forcing the values to be in the allowed list with the dropdown, when the validation might not apply.

Interested to hear what @adrianna-chang-shopify and @nvasilevski think. And anyone else actually.

So what are stateful constraints by the way? Lambdas that take the record? Also in: :some_method? We do have the object handy (although it's only initialized with its defaults obviously).

I guess they raise more issues, but I wonder why we wouldn't support that as well at this point.

Yeah, validations are designed to run within the context of the object with the values at the right place, so adding that limitation seems fair to me.

If someone wants to call a method, they can do so in a proc:

validates :model, inclusion: { in: ->() { MyClass.allowed_model_values } }

Do we need to deal with potential exceptions raised from the lambda? Before we were just getting a value from the task, now we're calling code, so I guess there's more potential for failure here.

Indeed, but if the code is throwing an exception, I think it's perfectly reasonable to let it raise unhandled

We should also document the dropdown feature (and the scope of the support for views with this PR) in the README as well. I guess that could be an indicator that the feature is too complex if we need too many words to explain it. Before: "if you have an inclusion validator on your parameter, if the allowed values is a Array, it will be used to build a dropdown". Now we'll need to add some more words.

Yeah, I can do that once we settle 👍🏼

Also just realized that if there's an if: or unless: on a validation, we're not respecting that (and ignoring the validation altogether), and always forcing the values to be in the allowed list with the dropdown, when the validation might not apply.

That's a valid concern, but it's also already true today. I don't think this PR makes the status quo any different.

I'd like to see MAX_INCLUSION_ITEMS made configurable, but I disagree with it defaulting to 0

Done

The tests are now failing, but I don't understand why. I don't see what it had to do with my PR 🤔

I rebased. Perhaps I picked up something?

Perhaps I picked up something?

It's reproducible on main so not related to the PR

Let's also add more docs to the README before this is merged

There doesn't seem to be any section in the README that exists where this can fit.

I suggest we merge this and tackle documentation separately.

@etiennebarrie fixed to support arity-one procs / lambdas appropriately, and I also tweaked this to support callable objects, since that is supported in the Validations::ResolveValue module you linked.